New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Timeprovider advance multiple invocations #4022

Merged

geeknoid merged 4 commits into dotnet:main from egil:3995-timeprovider-advance-multiple-invocations

Jun 7, 2023

+184 −80

Contributor

egil commented May 30, 2023 •

edited

Loading

This fixes #3995. These are the changes I have included:

The FakeTimeProviderTimer.Waiter now tracks when a WakeupTime was scheduled. It does this to ensure that the callbacks are invoked in the order they are scheduled in.
FakeTimeProviderTimer now triggers callbacks one at a time, and allows their callbacks to add reschedule themselves or influence other timers/create new timers.
FakeTimeProvider exposes an Epoch property, which represents the initial start time when the time provider was instantiated. It is a useful value to have available during testing, e.g. if you have to call SetUtcNow multiple times during a test, and the value you are setting is changed in relation to the start time/epoch. It tends to lead to easier-to-understand test code in my experience.
I also changed the ctor argument startTime on FakeTimeProvider to epoch to match the name of the added property.
Changed Advance() and SetUtcNow() to prevent moving time back.

Microsoft Reviewers: Open in CodeFlow


          Refactor tests for improved readability

758ce3c

ghost assigned egil

egil force-pushed the 3995-timeprovider-advance-multiple-invocations branch from 6e06690 to d1abbb4 Compare

May 30, 2023 22:18

Contributor Author

egil commented May 30, 2023

I didn't want to change too much of the existing implementation, but an alternative approach to implementing this I've used in my ManualTimeProvider is to only hold waiters in the FakeTimeProvider.Waiters collection in while their timer has a scheduled callback, and keep the waiters sorted by WakeupTime. That would eliminate the need for the Waiter.ScheduledOn property but would be a bigger change to the existing implementation.

egil removed their assignment

ghost assigned egil

sebastienros reviewed

View reviewed changes

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs Show resolved Hide resolved

RussKie reviewed

View reviewed changes

Member

RussKie left a comment

Few nits

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs Outdated Show resolved Hide resolved

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs Outdated Show resolved Hide resolved

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs Outdated Show resolved Hide resolved

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs Outdated Show resolved Hide resolved

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProviderTimer.cs Outdated Show resolved Hide resolved

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProviderTimer.cs Outdated Show resolved Hide resolved

RussKie added waiting-author-feedback 📭 waiting-on-team 👋 labels

ghost removed the waiting-author-feedback 📭 label

egil requested review from RussKie and sebastienros

June 5, 2023 09:44

egil commented

View reviewed changes

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs Outdated Show resolved Hide resolved

geeknoid reviewed

View reviewed changes

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs Outdated Show resolved Hide resolved

egil force-pushed the 3995-timeprovider-advance-multiple-invocations branch from debbca7 to 1ee526e Compare

June 5, 2023 19:42

joperezr assigned sebastienros

Member

RussKie commented Jun 5, 2023

I had a brief chat with @sebastienros, and it looks like I may have misinterpreted his comments. I am removing myself from this PR and letting @sebastienros and @geeknoid take over the reviews.

sebastienros reviewed

View reviewed changes

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs

    
            @@ -158,28 +166,39 @@ internal void RemoveWaiter(FakeTimeProviderTimer.Waiter waiter)
          
                      }

                  }

                  private List<FakeTimeProviderTimer.Waiter> GetWaitersToWake()

                  private FakeTimeProviderTimer.Waiter? TryGetWaiterToWake(DateTimeOffset targetNow)

Member

sebastienros Jun 5, 2023

Could it use the standard Try... pattern by being bool TryGetWaiterToWake(DateTimeOffset targetNow, out FakeTimeProviderTimer.Waiter?)

Contributor Author

egil Jun 6, 2023

Yes. But please indulge me, why?

I would use the common bool TryGetX(Foo foo, out Foo bar) in a public API surface since that is a very recognizable pattern in the .NET ecosystem. For private/internal stuff, I have changed to this pattern since we now have nullable reference types and pattern matching, I feel this version is more direct, clean, and doesn't involve out arguments. I am fairly certain that had these language features been around in the early days, the TryGetX pattern would not be the norm.

I do respect that you may have a coding standard in this repo you like to follow (I admit I didn't look at other projects in the repo, just this one) and I would change it regardless if you prefer the other style.

Contributor Author

egil commented Jun 6, 2023

@sebastienros / @geeknoid I've added you to my fork of the repo so you can push changes to this PR as you see fit.

geeknoid reviewed

View reviewed changes

src/Libraries/Microsoft.Extensions.TimeProvider.Testing/FakeTimeProvider.cs Outdated Show resolved Hide resolved

geeknoid approved these changes

View reviewed changes


          Expose Epoch/start time from FakeTimeProvider

ff5dd74

It is often useful to have the epoch/start time available
via the FakeTimeProvider during testing, as it provides a
stable offset (time-0) from which to advance or set a new
utc-now based off, e.g. to move to time-X on a timeline
starting at time-0.

Contributor Author

egil commented Jun 6, 2023 •

edited

Loading

Let me add the missing experimental attribute to the ctor and push a rewrite of the git history that bundles up the feedback into clean commits.

Ps. I do not have the permissions to merge the PR.

egil force-pushed the 3995-timeprovider-advance-multiple-invocations branch from e8010ee to 8e3c86e Compare

June 6, 2023 14:05

egil added 2 commits

June 6, 2023 14:19


          FakeTimeProvider invokes timer callbacks multiple times in single adv…

91916ff

…ance

fixes dotnet#3995

Reorder variable's in comparaison

Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>

remove unneeded comments

Squashed commit of the following:

commit 2f51f4063e2c0ec9042d425b50380f8c81ab6838
Author: Egil Hansen <egil@assimilated.dk>
Date:   Tue May 30 16:37:13 2023 +0000

    All changes


          Prevent Advance/SetUtcNow moving time backwards

145515a

egil force-pushed the 3995-timeprovider-advance-multiple-invocations branch from 8e3c86e to 145515a Compare

June 6, 2023 14:20

geeknoid merged commit 009b7bb into dotnet:main

ghost removed the waiting-on-team 👋 label

RussKie added this to the 8.0 Preview5 milestone

egil mentioned this pull request

FakeTimeProvider revamp. #4092

Merged

ghost locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet